Add Stop, Restart, and Force Rebuild menu items to Tsunami view#2483
Add Stop, Restart, and Force Rebuild menu items to Tsunami view#2483
Conversation
|
|
Co-authored-by: sawka <2722291+sawka@users.noreply.github.com>
Co-authored-by: sawka <2722291+sawka@users.noreply.github.com>
Co-authored-by: sawka <2722291+sawka@users.noreply.github.com>
WalkthroughA centralized private helper method Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–25 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@copilot ok, so just tested. the restart and force rebuild works great! but the restart does not actually trigger a restart -- because it is re-syncing... the synced state is running and so nothing happens. i'm thinking we should expose a "Stop WaveApp" item as well. You can use: and then for restart, it is just a stop, wait, and then call resync (which should restart it). |
Co-authored-by: sawka <2722291+sawka@users.noreply.github.com>
Added "Stop WaveApp" menu item and fixed restart logic. The "Restart WaveApp" now properly stops the controller, waits 300ms, then resyncs to restart it. Commit: 9cee89a Menu items now:
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
frontend/app/view/tsunami/tsunami.tsx (3)
90-93: Make resyncController async and await helperIf doControllerResync returns a Promise, await it for consistent flow and error bubbling.
- resyncController() { - this.doControllerResync(false, "resync", false); - } + async resyncController() { + await this.doControllerResync(false, "resync", false); + }
160-169: String-based filtering is brittle; prefer identifiers/rolesFiltering menu items by substring risks false positives and i18n issues. If ContextMenuItem has an id/role, filter on that instead. Otherwise, consider guarding only known labels or using a source flag from super.
170-189: Conditionally enable items and avoid double separators
- Disable “Stop” when not running; “Restart” can be enabled always.
- Insert separator only if there are remaining items and the next item isn’t already a separator.
- const tsunamiItems: ContextMenuItem[] = [ + const status = globalStore.get(this.shellProcFullStatus)?.shellprocstatus; + const running = status === "running"; + const tsunamiItems: ContextMenuItem[] = [ { label: "Stop WaveApp", - click: () => this.stopController(), + click: () => this.stopController(), + enabled: !!running, }, { label: "Restart WaveApp", - click: () => this.restartController(), + click: () => this.restartController(), }, { label: "Restart WaveApp and Force Rebuild", - click: () => this.restartAndForceRebuild(), + click: () => this.restartAndForceRebuild(), }, - { - type: "separator", - }, ]; - - return [...tsunamiItems, ...filteredItems]; + const needsSep = filteredItems.length > 0; + return needsSep + ? [...tsunamiItems, { type: "separator" }, ...filteredItems] + : [...tsunamiItems, ...filteredItems];
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/app/view/tsunami/tsunami.tsx(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build for TestDriver.ai
- GitHub Check: Analyze (go)
- GitHub Check: merge-gatekeeper
| private doControllerResync(forceRestart: boolean, logContext: string, triggerRestart: boolean = true) { | ||
| if (triggerRestart) { | ||
| if (globalStore.get(this.isRestarting)) { | ||
| return; | ||
| } | ||
| this.triggerRestartAtom(); | ||
| } | ||
| const prtn = RpcApi.ControllerResyncCommand(TabRpcClient, { | ||
| tabid: globalStore.get(atoms.staticTabId), | ||
| blockid: this.blockId, | ||
| forcerestart: false, | ||
| forcerestart: forceRestart, | ||
| }); | ||
| prtn.catch((e) => console.log("error controller resync", e)); | ||
| prtn.catch((e) => console.log(`error controller resync (${logContext})`, e)); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Return a Promise from doControllerResync and propagate errors
Callers can’t await the resync; errors are only logged. Make it async, return the underlying Promise, and rethrow to let callers handle.
- private doControllerResync(forceRestart: boolean, logContext: string, triggerRestart: boolean = true) {
+ private async doControllerResync(
+ forceRestart: boolean,
+ logContext: string,
+ triggerRestart: boolean = true
+ ): Promise<void> {
if (triggerRestart) {
if (globalStore.get(this.isRestarting)) {
return;
}
this.triggerRestartAtom();
}
- const prtn = RpcApi.ControllerResyncCommand(TabRpcClient, {
+ return RpcApi.ControllerResyncCommand(TabRpcClient, {
tabid: globalStore.get(atoms.staticTabId),
blockid: this.blockId,
forcerestart: forceRestart,
- });
- prtn.catch((e) => console.log(`error controller resync (${logContext})`, e));
+ }).catch((e) => {
+ console.log(`error controller resync (${logContext})`, e);
+ throw e;
+ });
}If RpcApi supports RpcOpts timeouts, consider adding a bounded timeout. Please confirm with:
🏁 Script executed:
#!/bin/bash
rg -n -C3 "ControllerResyncCommand\\s*\\(" \
&& rg -n -C3 "type\\s+RpcOpts|interface\\s+RpcOpts|export\\s+type\\s+RpcOpts"Length of output: 5675
Return a Promise from doControllerResync and propagate errors
The underlying RpcApi.ControllerResyncCommand already returns Promise<void>, but this method doesn't return or await it. Make the method async to return the Promise and rethrow errors so callers can handle them. Additionally, callers should pass RpcOpts with a timeout for robustness.
- private doControllerResync(forceRestart: boolean, logContext: string, triggerRestart: boolean = true) {
+ private async doControllerResync(
+ forceRestart: boolean,
+ logContext: string,
+ triggerRestart: boolean = true
+ ): Promise<void> {
if (triggerRestart) {
if (globalStore.get(this.isRestarting)) {
return;
}
this.triggerRestartAtom();
}
- const prtn = RpcApi.ControllerResyncCommand(TabRpcClient, {
+ return RpcApi.ControllerResyncCommand(TabRpcClient, {
tabid: globalStore.get(atoms.staticTabId),
blockid: this.blockId,
forcerestart: forceRestart,
- });
- prtn.catch((e) => console.log(`error controller resync (${logContext})`, e));
+ }).catch((e) => {
+ console.log(`error controller resync (${logContext})`, e);
+ throw e;
+ });
}Update all call sites (e.g., restartAndForceRebuild, restartController) to await the result. Consider adding rtopts parameter with timeout.
🤖 Prompt for AI Agents
In frontend/app/view/tsunami/tsunami.tsx around lines 75 to 88,
doControllerResync currently calls RpcApi.ControllerResyncCommand but neither
returns nor awaits its Promise; change the method signature to async and have it
return (or await and return) the RpcApi.ControllerResyncCommand Promise so
errors propagate to callers; rethrow errors instead of just logging (or let the
Promise reject) so callers can handle them; add an optional rtopts: RpcOpts
parameter (with a default timeout) and pass it into ControllerResyncCommand;
finally update all call sites (e.g., restartAndForceRebuild, restartController)
to await doControllerResync and handle/rethrow errors as appropriate.
| stopController() { | ||
| const prtn = RpcApi.ControllerStopCommand(TabRpcClient, this.blockId); | ||
| prtn.catch((e) => console.log("error stopping controller", e)); | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Stop should be awaitable; verify payload shape
Expose an async API and rethrow on failure so callers (and tests) can react. Also verify whether stop expects blockId or an object with tabid/blockid.
- stopController() {
- const prtn = RpcApi.ControllerStopCommand(TabRpcClient, this.blockId);
- prtn.catch((e) => console.log("error stopping controller", e));
- }
+ async stopController() {
+ try {
+ await RpcApi.ControllerStopCommand(TabRpcClient, this.blockId);
+ } catch (e) {
+ console.log("error stopping controller", e);
+ throw e;
+ }
+ }Run to confirm expected signature:
🏁 Script executed:
#!/bin/bash
# Locate definition and usages to confirm the data argument.
rg -n -C3 "ControllerStopCommand\\s*\\("Length of output: 3317
Make stopController() async and properly handle errors to allow caller reaction
The verification confirms the concern: RpcApi.ControllerStopCommand returns Promise<void> and is already awaitable, but the current implementation silently swallows errors. Notably, line 106 in the same file already demonstrates the correct pattern with await RpcApi.ControllerStopCommand(...) in a try block. The payload shape is correct—this.blockId is a string as expected by the backend.
Apply the suggested refactor to match the established pattern elsewhere in the codebase:
- stopController() {
- const prtn = RpcApi.ControllerStopCommand(TabRpcClient, this.blockId);
- prtn.catch((e) => console.log("error stopping controller", e));
- }
+ async stopController() {
+ try {
+ await RpcApi.ControllerStopCommand(TabRpcClient, this.blockId);
+ } catch (e) {
+ console.log("error stopping controller", e);
+ throw e;
+ }
+ }🤖 Prompt for AI Agents
In frontend/app/view/tsunami/tsunami.tsx around lines 94 to 98, change
stopController to be an async method, await
RpcApi.ControllerStopCommand(TabRpcClient, this.blockId) inside a try block,
catch any thrown error, log it with contextual message, and rethrow the error so
the caller can react to failures (matching the pattern used at line 106).
| async restartController() { | ||
| if (globalStore.get(this.isRestarting)) { | ||
| return; | ||
| } | ||
| this.triggerRestartAtom(); | ||
| const prtn = RpcApi.ControllerResyncCommand(TabRpcClient, { | ||
| tabid: globalStore.get(atoms.staticTabId), | ||
| blockid: this.blockId, | ||
| forcerestart: true, | ||
| }); | ||
| prtn.catch((e) => console.log("error controller resync (force restart)", e)); | ||
| try { | ||
| // Stop the controller first | ||
| await RpcApi.ControllerStopCommand(TabRpcClient, this.blockId); | ||
| // Wait a bit for the controller to fully stop | ||
| await new Promise((resolve) => setTimeout(resolve, 300)); | ||
| // Then resync to restart it | ||
| await RpcApi.ControllerResyncCommand(TabRpcClient, { | ||
| tabid: globalStore.get(atoms.staticTabId), | ||
| blockid: this.blockId, | ||
| forcerestart: false, | ||
| }); | ||
| } catch (e) { | ||
| console.log("error restarting controller", e); | ||
| } | ||
| } |
There was a problem hiding this comment.
Duplicate restart risk and brittle 300ms UI gating; use best‑effort stop and an in‑flight guard
isRestarting resets after 300ms, allowing overlapping restarts and misleading UI. Add a restartInFlight guard, keep UI pulse via triggerRestartAtom, proceed to resync even if stop says “already stopped,” and allow force rebuild reuse.
- async restartController() {
- if (globalStore.get(this.isRestarting)) {
- return;
- }
- this.triggerRestartAtom();
- try {
- // Stop the controller first
- await RpcApi.ControllerStopCommand(TabRpcClient, this.blockId);
- // Wait a bit for the controller to fully stop
- await new Promise((resolve) => setTimeout(resolve, 300));
- // Then resync to restart it
- await RpcApi.ControllerResyncCommand(TabRpcClient, {
- tabid: globalStore.get(atoms.staticTabId),
- blockid: this.blockId,
- forcerestart: false,
- });
- } catch (e) {
- console.log("error restarting controller", e);
- }
- }
+ async restartController(forceRebuild: boolean = false) {
+ if (this.restartInFlight) return;
+ this.restartInFlight = true;
+ this.triggerRestartAtom();
+ try {
+ // Best‑effort stop; continue even if already stopped.
+ try {
+ await RpcApi.ControllerStopCommand(TabRpcClient, this.blockId);
+ } catch (e) {
+ console.log("controller stop failed (continuing to resync)", e);
+ }
+ await new Promise((resolve) => setTimeout(resolve, 300));
+ await RpcApi.ControllerResyncCommand(TabRpcClient, {
+ tabid: globalStore.get(atoms.staticTabId),
+ blockid: this.blockId,
+ forcerestart: forceRebuild,
+ });
+ } catch (e) {
+ console.log("error restarting controller", e);
+ } finally {
+ this.restartInFlight = false;
+ }
+ }Add this field to the class (outside this hunk):
// Near other fields in TsunamiViewModel
private restartInFlight = false;🤖 Prompt for AI Agents
In frontend/app/view/tsunami/tsunami.tsx around lines 99 to 118, the restart
flow uses a brittle 300ms UI gating and can allow overlapping restarts; add a
private restartInFlight = false field on the TsunamiViewModel and update
restartController to return early if restartInFlight is true, set
restartInFlight = true at start and reset it in a finally block, keep calling
triggerRestartAtom() to preserve the UI pulse, attempt
RpcApi.ControllerStopCommand but treat failures (including “already stopped”) as
non-fatal and proceed to call RpcApi.ControllerResyncCommand regardless of stop
result, and remove the fixed 300ms timeout so you wait only for the stop Promise
(best‑effort) before resyncing.
| restartAndForceRebuild() { | ||
| this.doControllerResync(true, "force rebuild"); | ||
| } | ||
|
|
There was a problem hiding this comment.
Menu label says “Restart … and Force Rebuild” but code doesn’t stop first
Current implementation is a resync-only path. Align behavior with the label by reusing restartController(true).
- restartAndForceRebuild() {
- this.doControllerResync(true, "force rebuild");
- }
+ async restartAndForceRebuild() {
+ await this.restartController(true);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| restartAndForceRebuild() { | |
| this.doControllerResync(true, "force rebuild"); | |
| } | |
| async restartAndForceRebuild() { | |
| await this.restartController(true); | |
| } |
🤖 Prompt for AI Agents
In frontend/app/view/tsunami/tsunami.tsx around lines 120 to 123, the
restartAndForceRebuild method currently calls doControllerResync(true, "force
rebuild") which only resyncs; change it to call restartController(true) so the
controller is stopped and restarted with the force-rebuild flag. Replace the
doControllerResync call with restartController(true), preserving any
return/await behavior and updating any callers if the method signature/return
differs.
Adds settings menu items to the Tsunami app view for stopping and restarting the controller, with support for forcing rebuilds that bypass the build cache.
Changes
Added three menu items to
TsunamiViewModel.getSettingsMenuItems():ControllerStopCommandControllerResyncCommandwithforcerestart: true(bypasses build cache per line 228 intsunamicontroller.go)Refactored controller resync logic - extracted common code into
doControllerResync()helper with optionaltriggerRestartparameter to handle both user-initiated restarts and automatic mount syncImplemented proper restart behavior - "Restart WaveApp" now performs a stop → wait → resync sequence to ensure the controller actually restarts instead of just resyncing to the already-running state
Implementation Note
No backend changes required. The existing
CommandControllerResyncData.ForceRestartflag already controls cache bypass behavior in the tsunami controller's build logic, andControllerStopCommandis used for stopping the controller.Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.